[venstarthermostat] Add more channels provided by the local API #11305

raveydavies · 2021-09-25T20:38:08Z

<[Venstarthermostat] adding more functionality to match the local API>

<This Pull Request adds several enhancements to the Venstar Thermostat binding, as per enhancement issue 10823
#10823
Functionality added copies the functionality of the local API and is:
Fan Mode
Fan State
Schedule Mode
Schedule Part
Time Stamp for Run Time
Run Time in heat 1 mode
Run Time in heat 2 mode
Run Time in cool 1 mode
Run Time in cool 2 mode
Run Time in Aux 1 mode
Run Time in Aux 2 mode
Run Time in Free Cool mode

README is updated

<The updates have been tested in the Eclipse openHAB environment, and also on my Raspberry Pi with OpenHAB, using my thermostat device. I have a very simple heating system (central heating boiler), so could not test some channels with real equipment, but I have checked all channels update properly according to commands and the thermostat changes accordingly.

Signed-off-by: raveydavies <matthew.davies@skynet.be>

digitaldan

Hi, thanks so much for these improvements!!!! I'm scratching my head wondering why we did not at least implement Fan mode and state, as those seem pretty useful, so i think your changes will be well received!

I added some feedback, mostly small stuff.

A note about the Runtime history. It looks like the API is sending the last 7 days of runtime history. The timestamp for the last day is always right now, which makes sense as its the current runtime of each type until right now from the last entry, once midnight happens, it becomes yesterday's values (so the timestamp remains the same and its now not the last entry in that array).

I could see just having this last entry (current runtime) could be useful for historic reasons and graphing (so putting values in grafana, rrd, etc...). In this case i would update the documentation to make this more clear, as the timestamp is confusing why it's alway right now and that also the runtime values start at midnight. It confused me, which is why i went looking a little deeper.

We could also add channels for each of the last 7 days, this would be great for users wanting to display this info on their OH UI. If we could start over, adding channel groups might have been nice, but you can't mix them in now. I would vote for this option if you want to keep the runtime functionality. In this case your channels might look like

<channel id="day0Heat1Runtime" typeId="heat1Runtime"/>
<channel id="day1Heat1Runtime" typeId="heat1Runtime"/>
<channel id="day2Heat1Runtime" typeId="heat1Runtime"/>
....
<channel id="day0Cool2Runtime" typeId="cool2Runtime"/>
<channel id="day1Cool2Runtime" typeId="cool2Runtime"/>
<channel id="day2Cool2Runtime" typeId="cool2Runtime"/>
...

and so on. WDYT?

digitaldan · 2021-09-25T21:37:14Z

bundles/org.openhab.binding.venstarthermostat/README.md

+| scheduleModeRaw    | Number             | Current Schedule mode Raw (Read Only)| 0(Disabled) 1(Enabled)                         |
+| schedulePart       | String             | Current Schedule Part        |                                                        |
+| schedulePartRaw    | Number             | Schedule Part Raw (Read Only)|0(Morning) 1(Day) 2(Evening) 3 (Night) 255 (Inactive)   |
+| timestampRuntime   | DateTime           | Time Stamp of last RT update |Binding only looks at the last day runtime              |


Small nit, i would replace all the RT abbreviations with Runtime to make this more clear to users

OK I will update it. By the way, how does this formatting of a table in Markdown (in Eclipse) work? Is there a tutorial on it somewhere? I'm not familiar at all with this stuff, so I was kind of trying to fit RT into a column, probably not necessary :-).

So put everything you want into the table, don't worry about formatting too much, then before you push your changes, use http://markdowntable.com/ to have it format your table with the correct spacing and such. This is how i do it, others may have a different approach. Eclipse has a very poor markdown editor and previewer.

digitaldan · 2021-09-25T22:00:06Z

bundles/org.openhab.binding.venstarthermostat/src/main/resources/OH-INF/thing/thing-types.xml

+		<state readOnly="true" pattern="%d mins"/>
+	</channel-type>
+
+	<channel-type id="freecoolRuntime" advanced="false">


Suggested change

<channel-type id="freecoolRuntime" advanced="false">

<channel-type id="freeCoolRuntime" advanced="false">

Since "Free Cool" is two words, lets keep the camelCase pattern (and update the README as well).

OK will update it.

This still needs to be fixed as well

digitaldan · 2021-09-25T22:25:55Z

...in/java/org/openhab/binding/venstarthermostat/internal/handler/VenstarThermostatHandler.java

+        return z;
+    }
+
+    private int getHeat1Runtime(VenstarRuntime runtime) {


All these functions where you pass in the runtime are unnecessary, you can remove all of them and just call runtime.getHeat1() or other functions on the runtime object where you need it.

I was copying the approach that was in the code already. Yes, it seems a bit silly unless they are public functions.

digitaldan · 2021-09-25T22:29:16Z

...in/java/org/openhab/binding/venstarthermostat/internal/handler/VenstarThermostatHandler.java

@@ -338,23 +394,80 @@ private VenstarAwayMode getAwayMode() {
        return infoData.getAway();
    }

-    private void updateSettings(VenstarAwayMode away) {
+    private VenstarFanMode getFanMode() {


So i don't know why we have these private functions that just duplicate the functions inside objects we hold here. If you want to just remove all of these, that would be great (not sure why i did not catch them when i got involved) For example anywhere that is calling getFanMode() can just use infoData.getFanMode(). If these were public i might seen a reason for them, but they are not and its just redundant code.

Agree. I'll delete them, they are causing me a lot of unnecessary scrolling when I'm coding, it's driving me crazy.

digitaldan · 2021-09-25T22:39:36Z

...rmostat/src/main/java/org/openhab/binding/venstarthermostat/internal/dto/VenstarRuntime.java

+ * @author Matthew Davies - Class added to add more functionality to binding
+ */
+public class VenstarRuntime {
+    int ts;


timestamps should be stored as a long vs an int

OK. I will update it. Why is that? Coding convention because the number will eventually become very big?

yes, this will overrun a int in something like 2038 , which while a long while off, its still better to have consistency in coding conventions other wise we get a mess of ints and longs all over the place.

digitaldan · 2021-09-25T23:03:42Z

...in/java/org/openhab/binding/venstarthermostat/internal/handler/VenstarThermostatHandler.java

+
+    private ZonedDateTime getTimestampRuntime(VenstarRuntime runtime) {
+        ZonedDateTime z = ZonedDateTime.ofInstant(Instant.ofEpochSecond(runtime.getTimeStamp()),
+                ZoneId.systemDefault());


Actually it looks like this is reported in the user's timezone (or at least the time zone the device is set in) , so no need to convert to the system default time zone (or use GMT if you have to pass a timezone) . But this is actually more tricky as the runtime schedules are more complicated, i'll post that in another comment.

I was struggling with this due to lack of familiarity with timestamps, epoch, etc (I'm really an amateur....). What should I put as the function argument to Instant.ofEpochSecond() instead of "ZoneId.systemDefault()"? It's very confusing. I also noticed the time displayed in the channel was 2 hours ahead of my home time zone.

So this still needs to be resolved , or the time is always going to be off.

So this is kinda tricky, but i think you need to offset this back to UTC and then create a ZoneDate time. Dealing with time zones is always painful.

So maybe something like....

ZoneId zoneId = ZoneId.systemDefault(); ZonedDateTime now = LocalDateTime.now().atZone(zoneId); int diff = now.getOffset().getTotalSeconds(); ZonedDateTime z = ZonedDateTime.ofInstant(Instant.ofEpochSecond(runtime.getTimeStamp() -diff), zoneId);

raveydavies · 2021-09-26T12:01:51Z

Thanks for the comments!! I will take a look at them in detail later today when I have some free time. About the runtimes....I was also wondering about including them all, it should be fairly easy to do. What's a channel group?

digitaldan · 2021-09-26T16:51:56Z

Thanks for the comments!! I will take a look at them in detail later today when I have some free time. About the runtimes....I was also wondering about including them all, it should be fairly easy to do. What's a channel group?

See https://www.openhab.org/docs/developer/bindings/thing-xml.html#channel-groups , it allows you to define a group of channel types once and then refer to it many times (once for each day in this case). Unfortunately you either use ONLY channel groups OR individual channels, you can't mix them in a binding, so adding them would mean we break backwards compatibility for users. I think its ok, just means a little more copy and pasting in the thing xml file for each day, but in your code you can loop through the days and dynamically refer to the channels like (warning untested):

 private static final String CHANNEL_FMT = "day%d_%s";
          ....


            Collections.reverse(runtimes);
            for (int i = 0; i < 7; i++) {
                VenstarRuntime rt = null;
                if (i < runtimes.size()) {
                    rt = runtimes.get(i);
                }
                updateIfChanged(String.format(CHANNEL_FMT, i, CHANNEL_TIMESTAMP_RUNTIME),
                        rt == null ? UnDefType.UNDEF : new DateTimeType(getTimestampRuntime(lastRuntime)));
                updateIfChanged(String.format(CHANNEL_FMT, i, CHANNEL_HEAT1_RUNTIME),
                        rt == null ? UnDefType.UNDEF : new DecimalType(rt.getHeat1()));
                updateIfChanged(String.format(CHANNEL_FMT, i, CHANNEL_HEAT2_RUNTIME),
                        rt == null ? UnDefType.UNDEF : new DecimalType(rt.getHeat2()));
                updateIfChanged(String.format(CHANNEL_FMT, i, CHANNEL_COOL1_RUNTIME),
                        rt == null ? UnDefType.UNDEF : new DecimalType(rt.getCool1()));
                updateIfChanged(String.format(CHANNEL_FMT, i, CHANNEL_COOL2_RUNTIME),
                        rt == null ? UnDefType.UNDEF : new DecimalType(rt.getCool2()));
                updateIfChanged(String.format(CHANNEL_FMT, i, CHANNEL_AUX1_RUNTIME),
                        rt == null ? UnDefType.UNDEF : new DecimalType(rt.getAux1()));
                updateIfChanged(String.format(CHANNEL_FMT, i, CHANNEL_AUX2_RUNTIME),
                        rt == null ? UnDefType.UNDEF : new DecimalType(rt.getAux2()));
                updateIfChanged(String.format(CHANNEL_FMT, i, CHANNEL_FC_RUNTIME),
                        rt == null ? UnDefType.UNDEF : new DecimalType(rt.getFC()));
            }

I'm assuming there may not always be 7 days (like when its reset or something)
The channel constants would need to be changed to start with a capital letter of course to keep the camerlCase pattern.

raveydavies · 2021-09-26T20:24:36Z

Hi, thanks so much for these improvements!!!! I'm scratching my head wondering why we did not at least implement Fan mode and state, as those seem pretty useful, so i think your changes will be well received!

I added some feedback, mostly small stuff.

A note about the Runtime history. It looks like the API is sending the last 7 days of runtime history. The timestamp for the last day is always right now, which makes sense as its the current runtime of each type until right now from the last entry, once midnight happens, it becomes yesterday's values (so the timestamp remains the same and its now not the last entry in that array).

I could see just having this last entry (current runtime) could be useful for historic reasons and graphing (so putting values in grafana, rrd, etc...). In this case i would update the documentation to make this more clear, as the timestamp is confusing why it's alway right now and that also the runtime values start at midnight. It confused me, which is why i went looking a little deeper.

We could also add channels for each of the last 7 days, this would be great for users wanting to display this info on their OH UI. If we could start over, adding channel groups might have been nice, but you can't mix them in now. I would vote for this option if you want to keep the runtime functionality. In this case your channels might look like
<channel id="day0Heat1Runtime" typeId="heat1Runtime"/>
<channel id="day1Heat1Runtime" typeId="heat1Runtime"/>
<channel id="day2Heat1Runtime" typeId="heat1Runtime"/>
....
<channel id="day0Cool2Runtime" typeId="cool2Runtime"/>
<channel id="day1Cool2Runtime" typeId="cool2Runtime"/>
<channel id="day2Cool2Runtime" typeId="cool2Runtime"/>
...
and so on. WDYT?

Yes I agree with this as well. Do you think the timestamps are needed? Or would it be better to just have channels labeled "Today Heat1 Runtime", "Yesterday Heat1 Runtime", " 2 days ago Heat1 Runtime"........?

digitaldan · 2021-09-27T00:00:52Z

Yes I agree with this as well. Do you think the timestamps are needed?

It couldn't hurt, I would just mark the channel as "advanced" so it not shown by default in the UI

Or would it be better to just have channels labeled "Today Heat1 Runtime", "Yesterday Heat1 Runtime", " 2 days ago Heat1 Runtime"........?

Yes, exactly ! The labels and Readme can more clearly state "Today, Yesterday, 2 Days ago, 3 Days ago,......" and so on.

raveydavies · 2021-09-27T20:43:28Z

@digitaldan I'm working on the above changes. It will take me a couple of days. I'll commit the code when it's done and tested.

Signed-off-by: raveydavies <matthew.davies@skynet.be>

raveydavies · 2021-09-30T20:17:42Z

@digitaldan did my last commit come through OK? Something strange happened at my end, but to me it looks OK.

digitaldan · 2021-10-02T22:44:02Z

...in/java/org/openhab/binding/venstarthermostat/internal/handler/VenstarThermostatHandler.java

+
+            runtimeData = Objects.requireNonNull(gson.fromJson(response, VenstarRuntimeData.class));
+            List<VenstarRuntime> runtimes = runtimeData.getRuntimes();
+            String[] timestamps = { CHANNEL_TIMESTAMP_RUNTIME_DAY0, CHANNEL_TIMESTAMP_RUNTIME_DAY1,


If you don't want to dynamically reference a single set of base channels vars like CHANNEL_TIMESTAMP_RUNTIME_DAY + i thats fine, but theres a lot more code being copied and pasted around which is not my fav. If you do want to keep all these extra channels variables, arrays like these should be moved to VenstarThermostatBindingConstants or to the top of this file and be declared static final with the variable names in all caps (like other static final vars) so they are only created once.

I agree it's getting messy. I didn't like it when I programmed it, but I'm not sure how to resolve it (your CHANNEL_NAME+i won't work, will it?). I was actually wondering if it would be cleaner just to provide the runtime string, and let users do whatever they want with it (with some explanation of the String content in the documentation). I suspect anyone trying to do anything with this data will be quite "hardcore" anyway.

why would that not work? If you define a Variable like

public static final String CHANNEL_TIMESTAMP_RUNTIME_DAY = "timestampDay";

then in a loop set then channel String value to

updateIfChanged(CHANNEL_TIMESTAMP_RUNTIME_DAY + i, new DateTimeType(getTimestampRuntime(rt)));

Then the channel name is going to be

timestampDay0 timestampDay1 timestampDay2 ....

adding a String to a int results in concatenating the two together as essentially 2 strings. And you save yourself a whole lot of code to maintain. If we were doing this 1000's of time per second, then i agree having static tables would be more efficient , but if performance is not a issue, i would error on the side of maintainability.

Thanks!! Sounds good....I will update it. I guess I had somehow forgotten the variable values are strings.

What do you think about just providing the runtime string as it comes directly from the thermostat, as a simplification? I suppose as I've already done a lot of code around this, it's not worth reversing :-)

What do you think about just providing the runtime string as it comes directly from the thermostat,

Not sure what you mean?

I was just thinking it would be simpler to provide the raw json response from '.../query/runtimes' and let people do whatever they want with it with a JSON / Regex transformation operation. That would avoid all the extra channels and code we currently have (I don't like the fact the binding currently has 56 channels and 2 classes for this - it's messy :-O). A json probably wouldn't be too difficult to handle for people that are likely to use the data (it's not really core thermostat functionality). The thermostat also gives alarms in a similar format, which would again require multiple channels and classes unless a JSON response is used.

No, returning raw JSON is not a great choice here. I am fine with the extra channels and classes, you can mark them as advanced, and they won't show up by default in the UI, so it should not intimidate users. Returning in JSON will ensure very few people would ever use this. Again, if you dynamically create the channel names as we discussed a few times, then its just the thing XML which is long, but there are definitely bindings with more in them, so not out of the ordinary .

@digitaldan OK. I did the updates you suggested anyway. They are in my latest commit/push in this pull request.

...in/java/org/openhab/binding/venstarthermostat/internal/handler/VenstarThermostatHandler.java

digitaldan · 2021-10-02T22:58:39Z

...in/java/org/openhab/binding/venstarthermostat/internal/handler/VenstarThermostatHandler.java

+            String[] aux2Runtimes = { CHANNEL_AUX2_RUNTIME_DAY0, CHANNEL_AUX2_RUNTIME_DAY1, CHANNEL_AUX2_RUNTIME_DAY2,
+                    CHANNEL_AUX2_RUNTIME_DAY3, CHANNEL_AUX2_RUNTIME_DAY4, CHANNEL_AUX2_RUNTIME_DAY5,
+                    CHANNEL_AUX2_RUNTIME_DAY6 };
+            String[] fCRuntimes = { CHANNEL_FC_RUNTIME_DAY0, CHANNEL_FC_RUNTIME_DAY1, CHANNEL_FC_RUNTIME_DAY2,


nit

Suggested change

String[] fCRuntimes = { CHANNEL_FC_RUNTIME_DAY0, CHANNEL_FC_RUNTIME_DAY1, CHANNEL_FC_RUNTIME_DAY2,

String[] fcRuntimes = { CHANNEL_FC_RUNTIME_DAY0, CHANNEL_FC_RUNTIME_DAY1, CHANNEL_FC_RUNTIME_DAY2,

I thought it should be fC like the individual variable names. Why not?

b/c you clearly use fc as a single word here CHANNEL_FC_RUNTIME_DAY0 not CHANNEL_F_C_RUNTIME_DAY0 , and i agree FC is better then F_C

digitaldan · 2021-10-03T05:14:13Z

...in/java/org/openhab/binding/venstarthermostat/internal/handler/VenstarThermostatHandler.java

+
+    private ZonedDateTime getTimestampRuntime(VenstarRuntime runtime) {
+        ZonedDateTime z = ZonedDateTime.ofInstant(Instant.ofEpochSecond(runtime.getTimeStamp()),
+                ZoneId.systemDefault());


So this still needs to be resolved , or the time is always going to be off.

So this is kinda tricky, but i think you need to offset this back to UTC and then create a ZoneDate time. Dealing with time zones is always painful.

So maybe something like....

ZoneId zoneId = ZoneId.systemDefault(); ZonedDateTime now = LocalDateTime.now().atZone(zoneId); int diff = now.getOffset().getTotalSeconds(); ZonedDateTime z = ZonedDateTime.ofInstant(Instant.ofEpochSecond(runtime.getTimeStamp() -diff), zoneId);

Signed-off-by: raveydavies <matthew.davies@skynet.be>

raveydavies · 2021-10-03T20:17:56Z

@digitaldan I've made updates according to the comments, I think I addressed everything. Thanks for the coaching.....it's very useful for a self-taught programmer who never needed to code to any kind of standard or go through any peer review before :-)

digitaldan · 2021-10-03T20:23:39Z

I've made updates according to the comments, I think I addressed everything. Thanks for the coaching.....it's very useful for a self-taught programmer who never needed to code to any kind of standard or go through any peer review before :-)

No worries, these are nice enhancements, and i'm happy to help.

digitaldan

Hi , sorry for the delay, didn't catch you had made updates, see my review notes for latest round of feedback, thanks !

digitaldan · 2021-10-09T16:50:08Z

...in/java/org/openhab/binding/venstarthermostat/internal/handler/VenstarThermostatHandler.java

+        ZonedDateTime now = LocalDateTime.now().atZone(zoneId);
+        int diff = now.getOffset().getTotalSeconds();
+        ZonedDateTime z = ZonedDateTime.ofInstant(Instant.ofEpochSecond(runtime.getTimeStamp() - diff), zoneId);
+        // ZonedDateTime z = ZonedDateTime.ofInstant(Instant.ofEpochSecond(runtime.getTimeStamp()),


Please remove commented out code

digitaldan · 2021-10-09T16:51:53Z

...in/java/org/openhab/binding/venstarthermostat/internal/handler/VenstarThermostatHandler.java

@@ -93,10 +108,19 @@

    private static final int TIMEOUT_SECONDS = 30;
    private static final int UPDATE_AFTER_COMMAND_SECONDS = 2;
+    private static final String CHANNEL_TIMESTAMP_RUNTIME_DAY = "timestampDay";


Please put these in VenstarThermostatBindingConstants

digitaldan · 2021-10-09T16:52:58Z

...n/java/org/openhab/binding/venstarthermostat/internal/VenstarThermostatBindingConstants.java

+    public static final String CHANNEL_SCHEDULE_MODE_RAW = "scheduleModeRaw";
+    public static final String CHANNEL_SCHEDULE_PART = "schedulePart";
+    public static final String CHANNEL_SCHEDULE_PART_RAW = "schedulePartRaw";
+    public static final String CHANNEL_TIMESTAMP_RUNTIME_DAY0 = "timestampDay0";


Please remove all of the CHANNEL_xxx_xxx_Dayx strings as they are no longer referenced anywhere.

digitaldan · 2021-10-09T16:54:17Z

bundles/org.openhab.binding.venstarthermostat/README.md

+| scheduleMode       | String             | Current Schedule Mode                   |                                                        |
+| scheduleModeRaw    | Number             | Current Schedule mode Raw (Read Only)   | 0(Disabled) 1(Enabled)                                 |
+| schedulePart       | String             | Current Schedule Part                   |                                                        |
+| schedulePartRaw    | Number             | Schedule Part Raw (Read Only)           |0(Morning) 1(Day) 2(Evening) 3 (Night) 255 (Inactive)   |


Small nit, but will come up with other reviewers as well, run the table through a formatter or manually fix the spacing issue on this line.

digitaldan · 2021-10-09T16:57:20Z

bundles/org.openhab.binding.venstarthermostat/src/main/resources/OH-INF/thing/thing-types.xml

+		<state readOnly="true" pattern="%d mins"/>
+	</channel-type>
+
+	<channel-type id="freecoolRuntime" advanced="false">


This still needs to be fixed as well

Signed-off-by: raveydavies <matthew.davies@skynet.be>

raveydavies · 2021-10-09T19:34:54Z

@digitaldan Hi - I updated as per your review. I think I have finally got the freeCool camelCase right and the same everywhere!! I also made all the Runtime channels advanced. Everything seems to be working correctly with my thermostat. Let me know if anything else is needed. Thanks again.

raveydavies · 2021-10-10T08:36:38Z

@digitaldan just wondering if it's worth including the "alerts" provided by the API while we're at it. I think that's the only functionality which is not included now, apart from some data only applicable on commercial devices. WDYT?

digitaldan · 2021-10-10T14:41:51Z

HI looks good! There are some checkstyle warnings you can find in target/code-analysis/report.html that need to be fixed, but after that i'm good.

@digitaldan just wondering if it's worth including the "alerts" provided by the API while we're at it. I think that's the only functionality which is not included now, apart from some data only applicable on commercial devices. WDYT?

That sounds great, but let's open another PR for that and get this one merged in the meantime.

Signed-off-by: raveydavies <matthew.davies@skynet.be>

raveydavies · 2021-10-10T15:43:28Z

@digitaldan Thanks!! I can't see what the problem is with the checkstyle warnings about first author, in the Runtime and RuntimeData classes. What am I missing? I've corrected the other ones and pushed again.

digitaldan · 2021-10-15T18:47:18Z

Its very small formatting issues, should only take a min to fix. Its possible (likely) those existed before, but we were not using or checking checkstyle then. In any case this should be cleaned up for consistency as we do enforce this now.

raveydavies · 2021-10-15T20:08:38Z

I've fixed those. See my last commit. I previously also saw a warning about first author contribution but that's gone now. All should be fine with my last commit.

Signed-off-by: raveydavies <matthew.davies@skynet.be>

raveydavies · 2021-11-20T21:06:02Z

@lolodomo I think I addressed all your comments this time. Not sure what to do about the @NonNullByDefault for the serializer classes - the JsonDeserializer interface would need to be modified. Thanks for the detailed feedback, and for catching some of my really silly mistakes.

...c/main/java/org/openhab/binding/venstarthermostat/internal/dto/VenstarFanModeSerializer.java

lolodomo · 2021-11-21T10:42:23Z

@raveydavies : this is almost ok now. Please fix the two minor comments you did not handle properly (comments marked as not yet resolved) and consider the last one I just added about handling fromInt exception.
Regarding the kind oif channels to be added (fanState and scheduleMode), I would like to have @digitaldan opinion.

raveydavies · 2021-11-21T16:37:33Z

Oops. I missed those other calls to the fromInt method. I will correct them. What do you mean about the types of channels? What are you referring to?

lolodomo · 2021-11-21T17:22:26Z

What do you mean about the types of channels? What are you referring to?

I mean the kind of item attached to the channel. It can be a switch, a string, a number, ... You chose "String" while I think "Switch" would have been more appropriate. You just made a choice, this is not a bug.

Signed-off-by: raveydavies <matthew.davies@skynet.be>

raveydavies · 2021-12-03T21:07:04Z

@lolodomo updated again as per your review. Change the FanState Type to a switch, and added Exception Handling in the Serializer classes. Hope it's OK now....it works with my thermostat.

lolodomo

LGTM

lolodomo · 2021-12-04T09:49:38Z

Thank you @raveydavies

kaikreuzer · 2021-12-04T10:35:32Z

@lolodomo One quick wish when squashing+merging PRs: Please clean up the commit message when squashing to only keep the relevant information and a single sign-off line - otherwise they look like this. Thank you!

lolodomo · 2021-12-04T13:38:10Z

@kaikreuzer : that is noted for next times.

kaikreuzer · 2021-12-04T16:49:59Z

@lolodomo Many thanks. I know that this isn't documented anywhere and I should have mentioned this upfront, sorry for missing out on it.

raveydavies · 2021-12-04T19:45:17Z

Thanks @lolodomo @digitaldan for all for the help and feedback. It was tough for me to get that right, but we got there 😅. I learned a lot again.

raveydavies · 2021-12-06T09:53:41Z

@lolodomo I noticed a documentation error related to this PR. I forgot to change the item type for fan state to a Switch in the doc after changing it in the code. Is there a way to quickly update it or do I need to create another issue / PR?

lolodomo · 2021-12-06T11:05:16Z

Yes please create a new PR.

…1305) * Adding several functions to binding to mimic local API Signed-off-by: raveydavies <matthew.davies@skynet.be> * Adding functionality according to API Signed-off-by: raveydavies <matthew.davies@skynet.be> * Updating Read me with new capability Signed-off-by: raveydavies <matthew.davies@skynet.be> * Additional commit with requested changes to pull request Signed-off-by: raveydavies <matthew.davies@skynet.be> * Updates to address all comments on previous commit. Signed-off-by: raveydavies <matthew.davies@skynet.be> * Updates as requested in review. Signed-off-by: raveydavies <matthew.davies@skynet.be> * Corrections for check style warnings Signed-off-by: raveydavies <matthew.davies@skynet.be> * Updates to address feedback from lolodomo. Signed-off-by: raveydavies <matthew.davies@skynet.be> * Changes to address feedback from lolodomo's review Signed-off-by: raveydavies <matthew.davies@skynet.be> * FanState changed to Switch, Exception handling added as per review. Signed-off-by: raveydavies <matthew.davies@skynet.be> Signed-off-by: Nick Waterton <n.waterton@outlook.com>

…1305) * Adding several functions to binding to mimic local API Signed-off-by: raveydavies <matthew.davies@skynet.be> * Adding functionality according to API Signed-off-by: raveydavies <matthew.davies@skynet.be> * Updating Read me with new capability Signed-off-by: raveydavies <matthew.davies@skynet.be> * Additional commit with requested changes to pull request Signed-off-by: raveydavies <matthew.davies@skynet.be> * Updates to address all comments on previous commit. Signed-off-by: raveydavies <matthew.davies@skynet.be> * Updates as requested in review. Signed-off-by: raveydavies <matthew.davies@skynet.be> * Corrections for check style warnings Signed-off-by: raveydavies <matthew.davies@skynet.be> * Updates to address feedback from lolodomo. Signed-off-by: raveydavies <matthew.davies@skynet.be> * Changes to address feedback from lolodomo's review Signed-off-by: raveydavies <matthew.davies@skynet.be> * FanState changed to Switch, Exception handling added as per review. Signed-off-by: raveydavies <matthew.davies@skynet.be> Signed-off-by: Michael Schmidt <mi.schmidt.83@gmail.com>

…1305) * Adding several functions to binding to mimic local API Signed-off-by: raveydavies <matthew.davies@skynet.be> * Adding functionality according to API Signed-off-by: raveydavies <matthew.davies@skynet.be> * Updating Read me with new capability Signed-off-by: raveydavies <matthew.davies@skynet.be> * Additional commit with requested changes to pull request Signed-off-by: raveydavies <matthew.davies@skynet.be> * Updates to address all comments on previous commit. Signed-off-by: raveydavies <matthew.davies@skynet.be> * Updates as requested in review. Signed-off-by: raveydavies <matthew.davies@skynet.be> * Corrections for check style warnings Signed-off-by: raveydavies <matthew.davies@skynet.be> * Updates to address feedback from lolodomo. Signed-off-by: raveydavies <matthew.davies@skynet.be> * Changes to address feedback from lolodomo's review Signed-off-by: raveydavies <matthew.davies@skynet.be> * FanState changed to Switch, Exception handling added as per review. Signed-off-by: raveydavies <matthew.davies@skynet.be>

…1305) * Adding several functions to binding to mimic local API Signed-off-by: raveydavies <matthew.davies@skynet.be> * Adding functionality according to API Signed-off-by: raveydavies <matthew.davies@skynet.be> * Updating Read me with new capability Signed-off-by: raveydavies <matthew.davies@skynet.be> * Additional commit with requested changes to pull request Signed-off-by: raveydavies <matthew.davies@skynet.be> * Updates to address all comments on previous commit. Signed-off-by: raveydavies <matthew.davies@skynet.be> * Updates as requested in review. Signed-off-by: raveydavies <matthew.davies@skynet.be> * Corrections for check style warnings Signed-off-by: raveydavies <matthew.davies@skynet.be> * Updates to address feedback from lolodomo. Signed-off-by: raveydavies <matthew.davies@skynet.be> * Changes to address feedback from lolodomo's review Signed-off-by: raveydavies <matthew.davies@skynet.be> * FanState changed to Switch, Exception handling added as per review. Signed-off-by: raveydavies <matthew.davies@skynet.be> Signed-off-by: Andras Uhrin <andras.uhrin@gmail.com>

raveydavies added 3 commits September 25, 2021 20:54

Adding several functions to binding to mimic local API

147646d

Signed-off-by: raveydavies <matthew.davies@skynet.be>

Adding functionality according to API

3bf1a74

Signed-off-by: raveydavies <matthew.davies@skynet.be>

Updating Read me with new capability

415a47c

Signed-off-by: raveydavies <matthew.davies@skynet.be>

raveydavies requested a review from digitaldan as a code owner September 25, 2021 20:38

digitaldan changed the title ~~Venstar binding more functions issue enhancement 10823~~ [venstar] more functions issue enhancement 10823 Sep 25, 2021

digitaldan changed the title ~~[venstar] more functions issue enhancement 10823~~ [venstarthermostat] more functions issue enhancement 10823 Sep 25, 2021

digitaldan requested changes Sep 25, 2021

View reviewed changes

digitaldan added the work in progress A PR that is not yet ready to be merged label Sep 30, 2021

Additional commit with requested changes to pull request

2229467

Signed-off-by: raveydavies <matthew.davies@skynet.be>

raveydavies requested a review from digitaldan September 30, 2021 20:39

digitaldan requested changes Oct 3, 2021

View reviewed changes

Updates to address all comments on previous commit.

d37477e

Signed-off-by: raveydavies <matthew.davies@skynet.be>

digitaldan requested changes Oct 9, 2021

View reviewed changes

Updates as requested in review.

e2367d9

Signed-off-by: raveydavies <matthew.davies@skynet.be>

Corrections for check style warnings

583fe4a

Signed-off-by: raveydavies <matthew.davies@skynet.be>

digitaldan added the rebuild Triggers Jenkins PR build label Oct 15, 2021

Changes to address feedback from lolodomo's review

fef9759

Signed-off-by: raveydavies <matthew.davies@skynet.be>

lolodomo requested changes Nov 21, 2021

View reviewed changes

...c/main/java/org/openhab/binding/venstarthermostat/internal/dto/VenstarFanModeSerializer.java Outdated Show resolved Hide resolved

lolodomo added enhancement An enhancement or new feature for an existing add-on and removed awaiting feedback Awaiting feedback from the pull request author labels Nov 22, 2021

lolodomo mentioned this pull request Nov 25, 2021

Update VenstarThermostatBindingConstants.java #11638

Closed

FanState changed to Switch, Exception handling added as per review.

f1c81d3

Signed-off-by: raveydavies <matthew.davies@skynet.be>

lolodomo approved these changes Dec 4, 2021

View reviewed changes

lolodomo merged commit 6c6c93e into openhab:main Dec 4, 2021

lolodomo added this to the 3.2 milestone Dec 4, 2021

raveydavies deleted the venstar-binding-more-functions-issue-enhancement-10823 branch December 4, 2021 21:38

wborn mentioned this pull request Dec 18, 2021

[Venstar thermostat] Add more channels to the binding, which are available in the local API #10823

Closed

wborn changed the title ~~[venstarthermostat] more functions issue enhancement 10823~~ [venstarthermostat] Add more channels provided by the local API Dec 18, 2021

	<channel-type id="freecoolRuntime" advanced="false">
	<channel-type id="freeCoolRuntime" advanced="false">

	String[] fCRuntimes = { CHANNEL_FC_RUNTIME_DAY0, CHANNEL_FC_RUNTIME_DAY1, CHANNEL_FC_RUNTIME_DAY2,
	String[] fcRuntimes = { CHANNEL_FC_RUNTIME_DAY0, CHANNEL_FC_RUNTIME_DAY1, CHANNEL_FC_RUNTIME_DAY2,

[venstarthermostat] Add more channels provided by the local API #11305

[venstarthermostat] Add more channels provided by the local API #11305

Conversation

raveydavies commented Sep 25, 2021

digitaldan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raveydavies commented Sep 26, 2021

digitaldan commented Sep 26, 2021

raveydavies commented Sep 26, 2021

digitaldan commented Sep 27, 2021

raveydavies commented Sep 27, 2021

raveydavies commented Sep 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

digitaldan Oct 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raveydavies commented Oct 3, 2021

digitaldan commented Oct 3, 2021

digitaldan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raveydavies commented Oct 9, 2021

raveydavies commented Oct 10, 2021

digitaldan commented Oct 10, 2021

raveydavies commented Oct 10, 2021

digitaldan commented Oct 15, 2021

raveydavies commented Oct 15, 2021

raveydavies commented Nov 20, 2021

lolodomo commented Nov 21, 2021 • edited Loading

raveydavies commented Nov 21, 2021

lolodomo commented Nov 21, 2021

raveydavies commented Dec 3, 2021

lolodomo left a comment

Choose a reason for hiding this comment

lolodomo commented Dec 4, 2021 • edited Loading

kaikreuzer commented Dec 4, 2021

lolodomo commented Dec 4, 2021

kaikreuzer commented Dec 4, 2021

raveydavies commented Dec 4, 2021

raveydavies commented Dec 6, 2021

lolodomo commented Dec 6, 2021

digitaldan Oct 3, 2021 •

edited

Loading

lolodomo commented Nov 21, 2021 •

edited

Loading

lolodomo commented Dec 4, 2021 •

edited

Loading